Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

extract UploadSessionLister interface #4375

Merged
merged 6 commits into from
Dec 1, 2023
Merged

extract UploadSessionLister interface #4375

merged 6 commits into from
Dec 1, 2023

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Nov 29, 2023 •
edited
Loading

This is a little cleanup of the UploadProgress interface I had to introduce in #4148

The UploadSessionLister interface allows listing and filtering existing UploadSessions by id, processing or expired state. This can be used by the ocis cli to purge expired AND processed uploads, list uploads that have expired but are still processing etc.

The UploadSession Interface could be extended to include processing steps like the antivirus scan outcome etc, but currently that is stored by the processing service. This may need more thoughts before changing it. I think it makes sense to persist tho outcome of processing steps in the upload session metadata. Unfortunately, we currently store processing stata and scan outcome in the node metadata, not the upload metadata. This will change ofter merging #4148 , then we can think about if and where to persist the processing state ...

For now, I think this PR is ready.

@butonic butonic self-assigned this Nov 29, 2023
@update-docs Update Docs
Copy link

update-docs bot commented Nov 29, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@aduffeck
Copy link
Contributor

The TODOs and FIXMEs should be addressed or turned into issues before merging this in my opinion, but the general interface looks good to me.
How's the filtering of uploads gonna look like though? Does that become part of the ListUploads() call?

@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023 •
edited
Loading

I wonder if ListUploads() should take a filter. one by id to replace GetUploadProgress() and one by status to allow implementing cli tooling that allows listing currently ongoing uploads, uploads that are still in postprocessing to see which ones may have stalle etc. Hm, let me check ifg the graph API has anything interesting...

👀

hm graph uses uploadSessions to represent ongoing uploads ... and the properties expirationDateTime, nextExpectedRanges and uploadUrl are mostly covered by the current interface. ocis / reva actually knows about multiple protocols ... but that is nothing to expose via tha graph api ...

hm

@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

@aduffeck I completely changed the upload interface. I adopted UploadSessions to differentiate it from tus uploads. similar to tus.Info every UploedSession has a Purge() that can be used to completely remove the upload. I don't fully know if we need to emit an event, I think so, but I need to check in which cases.

The ListUploadSessions takes a filter that allows filtering the uploads by expired status, isprocessing flag or id. This allows all current usages and even more granular listing of uploads by the cli.

I also added the antivirus state, which is set during postprocessing. This can be used to list all not yet scanned uploads ...

your opinion?

@butonic butonic force-pushed the exctract-uploadmetadata-interface branch 2 times, most recently from 0ceef88 to 69ec64f Compare November 30, 2023 13:56
@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

tracking the tus upload-expires bug in #4379

@butonic butonic force-pushed the exctract-uploadmetadata-interface branch from 670b4c2 to e8b0c90 Compare November 30, 2023 15:11
@butonic butonic marked this pull request as ready for review November 30, 2023 15:17
@butonic butonic requested review from a team, labkode and glpatcern as code owners November 30, 2023 15:17
@aduffeck
Copy link
Contributor

@aduffeck I completely changed the upload interface. ...
your opinion?

lgtm, as discussed in zoom 👍

@butonic
extract upload manager interface
0587a11 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
iterate interface, add filter
87f5191 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
incorporate feedback
8743cda 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
add changelog
f37fe42 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic force-pushed the exctract-uploadmetadata-interface branch from 5837476 to f37fe42 Compare November 30, 2023 15:30
@butonic butonic changed the title extract upload manager interface extract UploadSessionLister interface Nov 30, 2023
kobergj
kobergj requested changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good but the linter is not happy

Additional Question: Did you run a full-ci pipeline on ocis to make sure these changes don't break ocis pipeline?

pkg/storage/uploads.go Show resolved Hide resolved
pkg/storage/uploads.go Outdated Show resolved Hide resolved
pkg/storage/uploads.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload.go Outdated Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/processing.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/processing.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/processing.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/processing.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/processing.go Outdated Show resolved Hide resolved
pkg/storage/uploads.go Show resolved Hide resolved
@butonic
make jklinter happy
f4e06ef 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

@kobergj I hope I made the linter happy ;-) will create an ocis bump right away

@butonic butonic mentioned this pull request Nov 30, 2023
Merged
@butonic
restore sending path in events
4db6635 
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic merged commit a389ddc into cs3org:edge Dec 1, 2023
9 checks passed
@butonic
Copy link
Contributor Author

butonic commented Dec 1, 2023

ocis pr was green, will rebase #4148 now

@butonic butonic deleted the exctract-uploadmetadata-interface branch December 1, 2023 12:22
@butonic butonic mentioned this pull request Dec 11, 2023
Merged
@micbar micbar mentioned this pull request Dec 12, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobergj kobergj kobergj approved these changes

@aduffeck aduffeck Awaiting requested review from aduffeck

@labkode labkode Awaiting requested review from labkode labkode is a code owner

@glpatcern glpatcern Awaiting requested review from glpatcern glpatcern is a code owner

Assignees

@butonic butonic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@butonic @aduffeck @kobergj